Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement config, recipe loader & recipe runner. #18

Merged
merged 16 commits into from
Aug 8, 2023

Conversation

BSchilperoort
Copy link
Contributor

@BSchilperoort BSchilperoort commented Aug 7, 2023

This PR implements a simple config loader, recipe loader, a recipe runner, and a CLI to run the recipes.

To review this PR: follow the instructions in docs/using_zampy.md.

  • Additionally, I have set up the config required for mkdocs. The docs can now be viewed with hatch run docs:serve
  • We can split up the recipe steps in the future, if a user only wants to re-run a single step.

@BSchilperoort BSchilperoort marked this pull request as ready for review August 7, 2023 13:26
docs/using_zampy.md Outdated Show resolved Hide resolved
docs/using_zampy.md Outdated Show resolved Hide resolved
docs/using_zampy.md Outdated Show resolved Hide resolved
docs/using_zampy.md Outdated Show resolved Hide resolved
src/zampy/recipe.py Outdated Show resolved Hide resolved
src/zampy/recipe.py Outdated Show resolved Hide resolved
Copy link
Member

@SarahAlidoost SarahAlidoost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BSchilperoort great to see the documentation is set up 👍 , thank you. please see my comments. Two more major issues:

  1. I was able to run the test_recipe and inspected the results using ncdump -h. It seems that the time and coordinates aren't correct. can you please check it? see:
ncdump -h output/test_recipe/era5_2020-2020.nc
netcdf era5_2020-2020 {
dimensions:
        time = 8040 ;
        latitude = 9 ;
        longitude = 7 ;
variables:
        float Wind_N(time, latitude, longitude) ;
                Wind_N:_FillValue = NaNf ;
                Wind_N:units = "meter_per_second" ;
                Wind_N:long_name = "10 metre V wind component" ;
                Wind_N:description = "" ;
        float Psurf(time, latitude, longitude) ;
                Psurf:_FillValue = NaNf ;
                Psurf:units = "pascal" ;
                Psurf:long_name = "Surface pressure" ;
                Psurf:standard_name = "surface_air_pressure" ;
                Psurf:description = "" ;
        double latitude(latitude) ;
                latitude:_FillValue = NaN ;
        double longitude(longitude) ;
                longitude:_FillValue = NaN ;
        int64 time(time) ;
                time:units = "hours since 2020-01-01 00:00:00" ;
                time:calendar = "proleptic_gregorian" ;

// global attributes:
                :Conventions = "ALMA" ;
                :history = "2023-08-08 06:11:32 GMT by grib_to_netcdf-2.25.1: /opt/ecmwf/mars-client/bin/grib_to_netcdf.bin -S param -o /cache/data4/adaptor.mars.internal-1691475090.8238995-25715-3-57d6b3c1-c339-4cf0-90bf-0630504e6dbf.nc /cache/tmp/57d6b3c1-c339-4cf0-90bf-0630504e6dbf-adaptor.mars.internal-1691475052.0872161-25715-5-tmp.grib" ;
}
ncdump -h output/test_recipe/eth_canopy_height_2020-2020.nc 
netcdf eth_canopy_height_2020-2020 {
dimensions:
        time = 1 ;
        latitude = 9 ;
        longitude = 7 ;
variables:
        float Hveg(time, latitude, longitude) ;
                Hveg:_FillValue = NaNf ;
                Hveg:AREA_OR_POINT = "Area" ;
                Hveg:units = "meter" ;
                Hveg:description = "" ;
        double latitude(latitude) ;
                latitude:_FillValue = NaN ;
        double longitude(longitude) ;
                longitude:_FillValue = NaN ;
        int64 time(time) ;
                time:units = "days since 2020-07-01 00:00:00" ;
                time:calendar = "proleptic_gregorian" ;

// global attributes:
                :Conventions = "ALMA" ;
}
  1. I see that you set up the documentation using mkdoc. However, read the docs or GitHub actions aren't still set up. Shall we make an issue for those?

src/zampy/cli.py Outdated Show resolved Hide resolved
@BSchilperoort
Copy link
Contributor Author

BSchilperoort commented Aug 8, 2023

  1. I was able to run the test_recipe and inspected the results using ncdump -h. It seems that the time and coordinates aren't correct. can you please check it? see:

Yeah we have to decide how to deal with the canopy height dataset, as technically it is only a snapshot and not a timeseries... I am not sure what is best. See #24

2. I see that you set up the documentation using mkdoc. However, read the docs or GitHub actions aren't still set up. Shall we make an issue for those?

I'll make an issue for that.

@BSchilperoort BSchilperoort mentioned this pull request Aug 8, 2023
3 tasks
Copy link
Member

@SarahAlidoost SarahAlidoost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BSchilperoort nice work on adding tests and thank you for addressing the comments. A suggestion about eth_canopy_height in the recipe example in the documentation: it would be nicer to remove it from there and when issue #24 is fixed, we add it again.

@BSchilperoort BSchilperoort merged commit 33918c4 into main Aug 8, 2023
12 checks passed
@BSchilperoort BSchilperoort deleted the simple-recipe-runner branch August 8, 2023 12:39
@sonarcloud
Copy link

sonarcloud bot commented Aug 8, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

87.1% 87.1% Coverage
0.0% 0.0% Duplication

@geek-yang
Copy link
Member

WoW! Very fast movement...😂 Thought I would take a look when all the tests were added. It is merged already. Nice work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants